Fix proactive scale up injecting fake pods for scheduling gated pods #8580
Conversation
k8s-ci-robot
added
release-note-none
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: abdelrahman882 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
size/M
abdelrahman882
force-pushed
the
proactive-scaleup-schgates
branch
from
286ba74 to
ff346b2
Compare
abdelrahman882
force-pushed
the
proactive-scaleup-schgates
branch
from
ff346b2 to
4627816
Compare
k8s-ci-robot
added
size/L
cluster-autoscaler/processors/podinjection/pod_injection_processor_test.go
Outdated
Show resolved
Hide resolved
abdelrahman882
force-pushed
the
proactive-scaleup-schgates
branch
from
4627816 to
030ad2c
Compare
abdelrahman882
force-pushed
the
proactive-scaleup-schgates
branch
from
030ad2c to
a281a40
Compare
| for _, podOwnerRef := range pod.OwnerReferences { | ||
| // SchedulingGated pods can't be unschedualable nor unprocessed nor scheduled so it is not expected | ||
| // to have them as group sample nor in pod count, so decreasing desiredReplicas by one is enough | ||
| if grp, found := groups[podOwnerRef.UID]; found { |
There was a problem hiding this comment.
This should only subtract when podOwnerRef.Controller != nil && *podOwnerRef.Controller to be in sync with updatePodGroups below. But actually, instead of adding replicas and then subtracting them here, wouldn't it suffice to pass scheduling gated pods to groupPods? We're already passing a combined list of scheduled and unschedulable pods there.
There was a problem hiding this comment.
tl;dr passed scheduling gated pods to groupPods with pods, they would be added to pod count and we wont inject fake pods for them
This should only subtract when podOwnerRef.Controller != nil && *podOwnerRef.Controller to be in sync with updatePodGroups below
Not really, I believe it would be redundant check because if the UID exists in the groups map then it refers to the same object that is pre checked to be controller, but will add as double check
But actually, instead of adding replicas and then subtracting them here, wouldn't it suffice to pass scheduling gated pods to groupPods? We're already passing a combined list of scheduled and unschedulable pods there.
We are not adding replicas and then subtracting them, we add twice to count and subtract from desired, The only reason I am doing that after grouping is to not do the listing and filtering to get gated pods in case we don't have any groups.
abdelrahman882
force-pushed
the
proactive-scaleup-schgates
branch
from
a281a40 to
341d253
Compare
k8s-ci-robot
added
size/M
| var podsToInject []*apiv1.Pod | ||
| allPods, err := ctx.AllPodLister().List() | ||
| if err != nil { | ||
| klog.Errorf("Failed to list all pods from all pod lister: %v", err) |
There was a problem hiding this comment.
This is redundant with the error returned - we'll end up logging the error twice. I'd just return the error and let the caller do the logging.
What type of PR is this?
/kind bug
What this PR does / why we need it:
Proactive scale up is listing the controllers, check the desired replicas and inject fake pods to proactively scale up even before these pods are created and marked as unschedulable.
Proactive scale up is using the following formula for the number of fake pods to inject per controller:
Number of fake pods = number of controller desired pods - (scheduled pods + unschedualable pods + unprocessed pods)
The problem here is that scheduling gated pods are not being excluded along with unschedualable and unprocessed so proactive scale up injects fake pods for these gated pods ignoring the condition. that happens each loop which leads to not scaling down that empty space.
This PR subtracts the number of pods with scheduling gates from the number of fake pods to inject.
Which issue(s) this PR fixes:
NONE
Special notes for your reviewer:
ctx.AllPodLister().List()should be using a lister, so we are not having extra api call here to list the pods rather we just get it from cache.PodInjectionPodListProcessorin case of any error filtering out scheduling gated pods, I choose to only log a warning and not return an error so it doesn't block the rest of the processors asCombinedPodListProcessorstops the processing in case any one returns an error, which means any following processor (all others) will not be executedDoes this PR introduce a user-facing change?